Support different input types to block handlers, validate manifest and mapping functions #516
Conversation
| @@ -317,6 +318,76 @@ ${abiFunctions | |||
| }, immutable.List()) | |||
| } | |||
|
|
|||
| static validateBlockFunctions(manifest, { resolveFile }) { | |||
There was a problem hiding this comment.
I forgot how hard it is to review JS code, thanks for the reminder.
Does dataSource really have a .getIn method? Sure! One must assume.
Also, it's interesting to see what looks like a mature and fully-fledged persistent immutable data structure library in JS.
There was a problem hiding this comment.
Yeah, immutable.js is rarely seen in the wild but it is mature. Unfortunately, it's rather unidiomatic, hence a lot of graph-cli is a little... odd.
src/subgraph.js
Outdated
| functionDeclaration.name.text === handler.get('handler') && | ||
| functionDeclaration.signature.parameters.length === 1 && | ||
| functionDeclaration.signature.parameters[0].name.text == | ||
| 'block' && |
There was a problem hiding this comment.
Is it really productive to require a specific parameter name? Is it backward compatible?
manifest-schema.graphql
Outdated
| @@ -42,6 +42,7 @@ type EthereumContractAbi { | |||
|
|
|||
| type EthereumBlockHandler { | |||
| handler: String! | |||
| input: String | |||
There was a problem hiding this comment.
What is the input expected to be? Also, can we add a validation test under tests/cli/validation/?
There was a problem hiding this comment.
The variants for input are Block, FullBlock and FullBlockWithReceipts with it defaulting to Block if nothing is specified.
Sure, I'll add a validation test. :)
There was a problem hiding this comment.
I don't like these names, they are not consistent with the spelling of other manifest fields. And they sound too coupled with the actual data types.
So, I'd prefer something like
blockFormat: block-only
blockFormat: block-with-transactions
blockFormat: block-with-receiptsPerhaps @Zerim can weigh in from a product / experience perspective.
There was a problem hiding this comment.
In general, I agree with @Jannis here. We don't use PascalCase anywhere else in the manifest for values that are defined by the protocol. I.e., we use things like ethereum/contract or ethereum/events.
I also think input is overly broad and that blockFormat or alternatively have some sort of handler>type: block-with-receipts or handler>type:block-with-receipts.... but we seem to have gone with the pattern of giving each handler type their own key in the manfiest rather than being a value.
There was a problem hiding this comment.
I see that in the network field of the manifest values use a hyphen like: mainnet, poa-core, poa-sokol. Not sure if present in other places.
fordN
force-pushed
the
ford/update-blockhandler-input-data
branch
from
1647377 to
7f97733
Compare
- Update field name: input -> blockFormat - Define and use string conversion from manifest formatted blockFormat values to struct names. Reporting an error with suggestions if an unsupported value is used.
fordN
force-pushed
the
ford/update-blockhandler-input-data
branch
from
7f97733 to
1a171a7
Compare
| @@ -23,6 +24,13 @@ const throwCombinedError = (filename, errors) => { | |||
| ) | |||
| } | |||
|
|
|||
| // Define conversions from a 'blockFormat' value in the manifest to its corresponding struct name | |||
| const blockFormatToStructName = immutable.Map({ | |||
There was a problem hiding this comment.
As we now have a separation of naming convention between blockFormat values in the manifest and their corresponding struct names, I've included a mapping between the two here. This gets used in validateBlockFunctions() to ensure the data structure used in the mapping matches what was defined in the manifest.
This PR contributes to the new feature that allows subgraph developers to choose between different Ethereum block structs to use as the input argument to a block handler mapping function. An optional
inputfield is now supported in the block handler definition in the manifest that is used to define the type of the input argument to the mapping function. Validation for the new field parses the Assemblyscript AST of the mappings file to ensure function signature of the block handler function matches its definition in the manifest.